Next release#1688
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR restructures loading skeleton CSS and markup across the application, standardizing skeleton fade-out duration to 50ms on most pages (10ms for settings), updates responsive grid layouts for settings and devices skeletons, removes inline min-height constraints from multiple tab containers, refines plugins table rendering and DataTables configuration, adds device connections metadata to MQTT Home Assistant discovery payloads, corrects a Telegram plugin config typo, enhances plugin column rendering, and introduces in-app navigation spinner triggering. ChangesSkeleton UI and Fade Timing Refactor
Plugins UI Updates and Navigation Interaction
Plugin Integration Fixes and Config Updates
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@front/plugins/_publisher_telegram/config.json`:
- Line 429: In the config.json file for the _publisher_telegram plugin, locate
the "string" property containing the Telegram bot token description. Replace the
word "cat" with "can" so the text reads "You can get one from..." instead of
"You cat get one from..." to correct the grammatical error in the BotFather link
description.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 148dd359-0893-4cd0-a34b-4bdab5c07b68
📒 Files selected for processing (6)
front/css/app.cssfront/php/templates/skel_settings.phpfront/php/templates/skel_tab_maint_logging.phpfront/plugins/_publisher_mqtt/mqtt.pyfront/plugins/_publisher_telegram/config.jsonfront/settings.php
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front/pluginsCore.php (1)
148-150: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUntrusted URL value is injected directly into HTML and link attributes.
Line 149 interpolates raw
valueintohrefand inner HTML, which allows markup/script injection via plugin data and keeps_blankopener risk. It also leaves malformed wrapper markup (<span>instead of</span>).🔒 Suggested hardening
case 'url': - value = `<span><a href="${value}" target="_blank">${value}</a><span>`; + { + const raw = `${value ?? ''}`.trim(); + const escapedText = $('<div/>').text(raw).html(); + try { + const parsed = new URL(raw, window.location.origin); + const isHttp = parsed.protocol === 'http:' || parsed.protocol === 'https:'; + if (!isHttp) throw new Error('Unsupported URL scheme'); + const safeHref = $('<div/>').text(parsed.href).html(); + value = `<span><a href="${safeHref}" target="_blank" rel="noopener noreferrer">${escapedText}</a></span>`; + } catch { + value = `<span>${escapedText}</span>`; + } + } break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front/pluginsCore.php` around lines 148 - 150, In the URL case block, the raw value variable is being directly interpolated into both the href attribute and the link's inner HTML without any escaping or sanitization, which creates a markup and script injection vulnerability. Fix this by properly escaping the value before interpolation, ensuring it's safe for use in both HTML content and attributes. Additionally, correct the malformed closing tag that reads <span> instead of </span>, and add the rel="noopener noreferrer" attribute to the anchor element to prevent opener window access risks when using target="_blank".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@front/css/app.css`:
- Around line 2676-2680: The border declaration in the "bordered containers"
rule uses `none` as the border style, which disables the border rendering for
all the skeleton container classes (.skel-overview-panel, .skel-section,
.skel-plugin-block, .skel-chart-box, .skel-table-box, .skel-detail-info-panel,
.skel-detail-form, .skel-tabs-bar). Replace the `none` value with a valid border
style such as `solid` in the border shorthand property so that the borders are
actually visible using the --skel-border color variable.
In `@front/pluginsCore.php`:
- Line 664: The `ordering: true` setting on line 664 enables sorting on all
plugin config columns, but the DataTable only retrieves a fixed subset of fields
from the GraphQL query. When users attempt to sort by columns not in the
returned dataset, sorting silently proceeds with None values (appearing last)
instead of producing meaningful results. To fix this, either restrict the
`ordering` flag to only be enabled for columns that are guaranteed to be
returned by the GraphQL query (the fixed subset including index, plugin,
objectPrimaryId, objectSecondaryId, dateTimeCreated, dateTimeChanged,
watchedValue1-4, status, extra, userData, foreignKey, syncHubNodeName,
helpVal1-4, objectGuid), or implement validation logic that checks each declared
plugin column definition against the actual query response fields before
allowing it to be sortable.
In `@front/settings.php`:
- Around line 552-556: The showSettingsSkeleton() function is using fadeOut()
before showing the element, which causes flickering when loading retries occur.
Replace the fadeOut() animation with a direct show() call or use fadeIn() if
animation is needed, so the skeleton element displays immediately without first
animating to a hidden state.
---
Outside diff comments:
In `@front/pluginsCore.php`:
- Around line 148-150: In the URL case block, the raw value variable is being
directly interpolated into both the href attribute and the link's inner HTML
without any escaping or sanitization, which creates a markup and script
injection vulnerability. Fix this by properly escaping the value before
interpolation, ensuring it's safe for use in both HTML content and attributes.
Additionally, correct the malformed closing tag that reads <span> instead of
</span>, and add the rel="noopener noreferrer" attribute to the anchor element
to prevent opener window access risks when using target="_blank".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 615a3a2b-d3bb-47cf-b8ad-bce5f551601c
📒 Files selected for processing (23)
front/css/app.cssfront/css/dark-patch.cssfront/css/system-dark-patch.cssfront/deviceDetails.phpfront/deviceDetailsEdit.phpfront/deviceDetailsEvents.phpfront/deviceDetailsPresence.phpfront/deviceDetailsSessions.phpfront/devices.phpfront/events.phpfront/maintenance.phpfront/php/templates/skel_devices.phpfront/php/templates/skel_presence.phpfront/php/templates/skel_tab_plugins_table.phpfront/plugins/_publisher_telegram/config.jsonfront/pluginsCore.phpfront/presence.phpfront/report.phpfront/settings.phpfront/systeminfo.phpfront/systeminfoInitCheck.phpfront/userNotifications.phpfront/workflowsCore.php
✅ Files skipped from review due to trivial changes (15)
- front/workflowsCore.php
- front/deviceDetailsPresence.php
- front/php/templates/skel_presence.php
- front/php/templates/skel_tab_plugins_table.php
- front/report.php
- front/maintenance.php
- front/systeminfoInitCheck.php
- front/deviceDetailsEdit.php
- front/systeminfo.php
- front/userNotifications.php
- front/presence.php
- front/deviceDetailsEvents.php
- front/plugins/_publisher_telegram/config.json
- front/devices.php
- front/deviceDetailsSessions.php
| paging: true, | ||
| searching: true, | ||
| ordering: false, | ||
| ordering: true, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Collect column names declared in plugin config files.
python - <<'PY'
import json, pathlib, sys
cfg_files = list(pathlib.Path('.').rglob('front/plugins/**/config.json'))
declared = set()
for p in cfg_files:
try:
data = json.loads(p.read_text(encoding='utf-8'))
except Exception:
continue
defs = data.get('database_column_definitions') or data.get('data', {}).get('database_column_definitions') or []
for d in defs:
col = d.get('column')
if col:
declared.add(col)
print("Declared columns:", len(declared))
for c in sorted(declared):
print(c)
PY
# 2) Print the GraphQL-selected fields from pluginsCore.php for manual contract check.
rg -n -A20 -B5 'query PluginData' front/pluginsCore.phpRepository: netalertx/NetAlertX
Length of output: 1398
🏁 Script executed:
#!/bin/bash
# Get context around line 664 and understand the table configuration
sed -n '650,680p' front/pluginsCore.php | cat -nRepository: netalertx/NetAlertX
Length of output: 1716
🏁 Script executed:
#!/bin/bash
# Find where the table columns are defined to understand what can be sorted
rg -n "columns:" front/pluginsCore.php -A 50 | head -100Repository: netalertx/NetAlertX
Length of output: 3862
🏁 Script executed:
#!/bin/bash
# Check if there's any validation or filtering of sort fields before sending to GraphQL
rg -n "sort\." front/pluginsCore.php -B 3 -A 3Repository: netalertx/NetAlertX
Length of output: 382
🏁 Script executed:
#!/bin/bash
# Examine the GraphQL schema/type definitions to see what fields support sorting
fd -i graphql front/ | head -20Repository: netalertx/NetAlertX
Length of output: 192
🏁 Script executed:
#!/bin/bash
# Examine the GraphQL query handler to understand sort parameter support
cat front/php/server/query_graphql.php | head -150Repository: netalertx/NetAlertX
Length of output: 2734
🏁 Script executed:
#!/bin/bash
# Search for sort parameter handling in GraphQL queries
rg -n "sort" front/php/server/query_graphql.php -B 2 -A 5Repository: netalertx/NetAlertX
Length of output: 157
🏁 Script executed:
#!/bin/bash
# Check for GraphQL type definitions that might define sortable fields
fd -type f -name "*.php" front/php/server | xargs grep -l "sortableBy\|sortField\|orderBy" 2>/dev/null || echo "No matches found"Repository: netalertx/NetAlertX
Length of output: 345
🏁 Script executed:
#!/bin/bash
# Look at the postPluginGraphQL function to understand the complete request/response flow
rg -n "postPluginGraphQL" front/pluginsCore.php -A 40Repository: netalertx/NetAlertX
Length of output: 3555
🏁 Script executed:
#!/bin/bash
# Search for GraphQL resolver/schema files
find front -name "*.js" -o -name "*.ts" | xargs grep -l "PluginData\|pluginsEvents\|pluginsHistory" 2>/dev/null | head -10Repository: netalertx/NetAlertX
Length of output: 157
🏁 Script executed:
#!/bin/bash
# Look for GraphQL implementation in the backend
find . -path ./node_modules -prune -o -type f \( -name "*.js" -o -name "*.py" \) -print | xargs grep -l "PluginQueryOptionsInput\|sort.*field" 2>/dev/null | grep -v node_modules | head -10Repository: netalertx/NetAlertX
Length of output: 485
🏁 Script executed:
#!/bin/bash
# Check if backend GraphQL has schema definitions
ls -la | grep -i graph
find . -maxdepth 3 -type d -name "*graph*" 2>/dev/nullRepository: netalertx/NetAlertX
Length of output: 157
🏁 Script executed:
#!/bin/bash
# Search for backend implementation directory structure
ls -la back/ 2>/dev/null || ls -la backend/ 2>/dev/null || ls -la server/ 2>/dev/null || echo "No backend dirs found"Repository: netalertx/NetAlertX
Length of output: 818
🏁 Script executed:
#!/bin/bash
# Check GraphQL types to see what fields are sortable
head -200 server/api_server/graphql_types.pyRepository: netalertx/NetAlertX
Length of output: 10751
🏁 Script executed:
#!/bin/bash
# Look for sort handling in GraphQL endpoint
rg -n "sort" server/api_server/graphql_endpoint.py -B 3 -A 5 | head -100Repository: netalertx/NetAlertX
Length of output: 2274
🏁 Script executed:
#!/bin/bash
# Check GraphQL helpers for sort field validation
cat server/api_server/graphql_helpers.py | head -150Repository: netalertx/NetAlertX
Length of output: 4584
🏁 Script executed:
#!/bin/bash
# Search for PluginQueryOptionsInput type definition
rg -n "PluginQueryOptionsInput" server/api_server/ -A 20Repository: netalertx/NetAlertX
Length of output: 6498
Sorting on undeclared columns won't cause GraphQL errors, but may produce unexpected results.
The review concern is partially incorrect. Enabling ordering: true (line 664) allows sorting by any column from plugin config definitions. However, unlike the original comment suggests, the GraphQL backend accepts all sort field names without validation—it doesn't reject unknown fields or return errors.
When a sort is applied to a field not present in the backend data, the resolver calls x.get(field) which returns None, and sorting proceeds with None values ordered last. The table loads successfully; it doesn't fail or appear empty from GraphQL errors.
However, a legitimate concern remains: plugin columns declared in configs (27 total) can be sorted, but the DataTable only queries a fixed subset of fields (index plugin objectPrimaryId objectSecondaryId dateTimeCreated dateTimeChanged watchedValue1-4 status extra userData foreignKey syncHubNodeName helpVal1-4 objectGuid). If a sortable column isn't in this returned dataset, users experience unexpected sort behavior (sorting by effectively empty values) rather than a clear failure.
Consider restricting sortable columns to only those returned by the GraphQL query, or validate declared column definitions against the query response fields.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@front/pluginsCore.php` at line 664, The `ordering: true` setting on line 664
enables sorting on all plugin config columns, but the DataTable only retrieves a
fixed subset of fields from the GraphQL query. When users attempt to sort by
columns not in the returned dataset, sorting silently proceeds with None values
(appearing last) instead of producing meaningful results. To fix this, either
restrict the `ordering` flag to only be enabled for columns that are guaranteed
to be returned by the GraphQL query (the fixed subset including index, plugin,
objectPrimaryId, objectSecondaryId, dateTimeCreated, dateTimeChanged,
watchedValue1-4, status, extra, userData, foreignKey, syncHubNodeName,
helpVal1-4, objectGuid), or implement validation logic that checks each declared
plugin column definition against the actual query response fields before
allowing it to be sortable.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front/pluginsCore.php (1)
152-165: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEncode URL values and improve link accessibility.
The
url_http_httpscase insertsvaluedirectly intohrefattributes without URL encoding. Ifvaluecontains spaces, quotes, or special characters, it could break the HTML structure or enable injection attacks. Additionally, displaying only lock icons without visible text or tooltips makes it unclear what URL the user is navigating to.🔒 Proposed fix to add URL encoding and accessibility
case 'url_http_https': - value = `<span> - <a href="http://${value}" target="_blank"> - <i class="fa fa-lock-open "></i> - </a> - / - <a href="https://${value}" target="_blank"> - <i class="fa fa-lock "></i> - </a> - </span>`; + value = `<span> + <a href="http://${encodeURIComponent(value)}" target="_blank" title="http://${encodeSpecialChars(value)}"> + <i class="fa fa-lock-open"></i> + </a> + / + <a href="https://${encodeURIComponent(value)}" target="_blank" title="https://${encodeSpecialChars(value)}"> + <i class="fa fa-lock"></i> + </a> + </span>`; break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front/pluginsCore.php` around lines 152 - 165, In the `url_http_https` case, the `value` variable is being directly inserted into the href attributes for both HTTP and HTTPS links without URL encoding, which could break the HTML or enable injection attacks if the value contains special characters or quotes. Additionally, the links only display lock icons without any visible text or accessibility attributes, making it unclear what URL the user is navigating to. Fix this by URL encoding the `value` using encodeURIComponent before inserting it into both `href="http://${value}"` and `href="https://${value}"` attributes, and add title attributes or visible text labels to the anchor tags to improve accessibility and user clarity about the destination URLs.
🧹 Nitpick comments (1)
front/js/common.js (1)
957-971: 📐 Maintainability & Code Quality | 🔵 TrivialConsider the design implications of double-spinner calls and hash-navigation exclusion.
The click handler triggers
showSpinner()for in-app navigations, and destination pages (e.g.,events.php,deviceDetails.php) also callshowSpinner()on load. While this creates two successive calls, the pattern appears intentional: the first call initiates feedback while the old page unloads, and the second call (documented inevents.phpas "Show initial page loading spinner") ensures visual continuity on the new page. The animations won't overlap since the first spinner is interrupted during page navigation.The pathname+search comparison also excludes hash changes, so links like
devices.php#my_deviceswon't trigger the spinner when already ondevices.php. This behavior likely serves in-page navigation (e.g., scrolling to sections), but creates inconsistent UX if users expect spinner feedback for all navigations.These appear to be intentional design choices rather than bugs. Consider documenting the intent and confirming the pattern works without visual flicker during page transitions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front/js/common.js` around lines 957 - 971, Add inline documentation comments to the click handler function that explains the intentional design pattern: clarify that the first showSpinner() call provides initial feedback as the old page unloads while destination pages trigger a second showSpinner() call for visual continuity, and note that this double-call pattern is deliberate to ensure smooth transitions without overlapping animations. Additionally, document why hash-based navigation (like links to `#my_devices`) are intentionally excluded from triggering the spinner by adding a comment explaining that the pathname+search comparison intentionally excludes hash changes to support in-page navigation scenarios. Consider adding a note confirming that this pattern has been tested and produces no visual flicker during page transitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@front/php/templates/header.php`:
- Line 366: The Maintenance menu link at line 366 in the header.php file has
lost its onclick handler, leaving only href="#", which makes the top-level menu
item non-functional. Restore the onclick="openUrl(['./maintenance.php'])"
handler to the anchor tag for the Maintenance menu item to match the pattern
used in the Settings menu link (line 394) and Devices menu link (line 291),
ensuring users can navigate directly to the maintenance.php page and making the
navigation pattern consistent across all treeview menu items.
- Line 291: The anchor tag in header.php with the `openUrl()` onclick handler
does not prevent default link behavior, causing both the onclick function and
the href to be executed, resulting in double navigation. Fix this by either
adding `return false;` to the end of the onclick attribute in the anchor tag so
it becomes `onclick="openUrl([......]); return false;"` OR modify the
`openUrl()` function in common.js (lines 691-713) to return false and prevent
the default link behavior, then update the onclick to use `return
openUrl([......])`
---
Outside diff comments:
In `@front/pluginsCore.php`:
- Around line 152-165: In the `url_http_https` case, the `value` variable is
being directly inserted into the href attributes for both HTTP and HTTPS links
without URL encoding, which could break the HTML or enable injection attacks if
the value contains special characters or quotes. Additionally, the links only
display lock icons without any visible text or accessibility attributes, making
it unclear what URL the user is navigating to. Fix this by URL encoding the
`value` using encodeURIComponent before inserting it into both
`href="http://${value}"` and `href="https://${value}"` attributes, and add title
attributes or visible text labels to the anchor tags to improve accessibility
and user clarity about the destination URLs.
---
Nitpick comments:
In `@front/js/common.js`:
- Around line 957-971: Add inline documentation comments to the click handler
function that explains the intentional design pattern: clarify that the first
showSpinner() call provides initial feedback as the old page unloads while
destination pages trigger a second showSpinner() call for visual continuity, and
note that this double-call pattern is deliberate to ensure smooth transitions
without overlapping animations. Additionally, document why hash-based navigation
(like links to `#my_devices`) are intentionally excluded from triggering the
spinner by adding a comment explaining that the pathname+search comparison
intentionally excludes hash changes to support in-page navigation scenarios.
Consider adding a note confirming that this pattern has been tested and produces
no visual flicker during page transitions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4ceefc85-74ab-4a6d-8011-56d59f710433
📒 Files selected for processing (4)
front/js/common.jsfront/php/templates/header.phpfront/pluginsCore.phpfront/settings.php
🚧 Files skipped from review as they are similar to previous changes (1)
- front/settings.php
|
|
||
| <li class=" treeview <?php if (in_array (basename($_SERVER['SCRIPT_NAME']), array('devices.php', 'deviceDetails.php') ) ){ echo 'active menu-open'; } ?>"> | ||
| <a href="#" onclick="openUrl(['./devices.php', './deviceDetails.php'])"> | ||
| <a href="devices.php" onclick="openUrl(['./devices.php', './deviceDetails.php'])"> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find and examine the openUrl function to check for preventDefault() or return false.
# Search for openUrl function definition
rg -n 'function\s+openUrl' --type=js -A 20
# Also check for arrow function or const assignment
rg -n '(const|let|var)\s+openUrl\s*=' --type=js -A 20Repository: netalertx/NetAlertX
Length of output: 157
🏁 Script executed:
# Search for openUrl in all file types
rg -n 'openUrl' --type=php -A 5
# Also search in HTML/template files
rg -n 'openUrl' --type=html -A 5
# Check if there's a script directory with JS files using different naming
find front -type f -name "*.js" | head -20
# Also search without type restriction to catch all definitions
rg -n 'openUrl\s*=' -A 10 | head -50Repository: netalertx/NetAlertX
Length of output: 425
🏁 Script executed:
# Read the header.php file to see context around lines 291 and 366
wc -l front/php/templates/header.phpRepository: netalertx/NetAlertX
Length of output: 192
🏁 Script executed:
# Read relevant sections of header.php - around line 291 and 366
cat -n front/php/templates/header.php | sed -n '280,300p'
cat -n front/php/templates/header.php | sed -n '360,375p'Repository: netalertx/NetAlertX
Length of output: 2511
🏁 Script executed:
# Search for openUrl definition across entire repository
rg -n 'openUrl' -C 3 | head -100Repository: netalertx/NetAlertX
Length of output: 157
🏁 Script executed:
# Search more broadly for openUrl - case insensitive, any context
rg -i 'openUrl'
# Also list all JS files in the front directory
find front -type f \( -name "*.js" -o -name "*.jsx" \)Repository: netalertx/NetAlertX
Length of output: 157
🏁 Script executed:
# Check if header.php contains inline scripts or function definitions
grep -n '<script' front/php/templates/header.php
# Search for any function that handles URL opening or navigation
rg -n 'function.*Url|onClick|openUrl|forceLoadUrl' -i | head -40Repository: netalertx/NetAlertX
Length of output: 1649
🏁 Script executed:
# List JS files in js directory
ls -la front/js/ | head -20
# Search specifically in js directory
rg -n 'openUrl' front/js/
# Also search for forceLoadUrl to understand the pattern
rg -n 'forceLoadUrl' front/js/ -A 5Repository: netalertx/NetAlertX
Length of output: 1680
🏁 Script executed:
# Read the openUrl function implementation in detail
cat -n front/js/common.js | sed -n '691,716p'Repository: netalertx/NetAlertX
Length of output: 1068
The Devices link will cause double navigation - openUrl() does not prevent default link behavior.
Line 291: <a href="devices.php" onclick="openUrl(['./devices.php', './deviceDetails.php'])"> creates a functional issue. The openUrl() function in front/js/common.js:691-713 checks if the current URL matches any entry in the array, but it does not call preventDefault() or return false. When clicked:
- The onclick handler executes
openUrl(), which either redirects or does nothing - The browser still follows the
href="devices.php"link regardless
This causes redundant navigation. Add return false; to the onclick attribute or modify openUrl() to return false to prevent the default link behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@front/php/templates/header.php` at line 291, The anchor tag in header.php
with the `openUrl()` onclick handler does not prevent default link behavior,
causing both the onclick function and the href to be executed, resulting in
double navigation. Fix this by either adding `return false;` to the end of the
onclick attribute in the anchor tag so it becomes `onclick="openUrl([......]);
return false;"` OR modify the `openUrl()` function in common.js (lines 691-713)
to return false and prevent the default link behavior, then update the onclick
to use `return openUrl([......])`
| <!-- Maintenance menu item --> | ||
| <li class=" treeview <?php if (in_array (basename($_SERVER['SCRIPT_NAME']), array('maintenance.php') ) ){ echo 'active menu-open'; } ?>"> | ||
| <a href="#" onclick="openUrl(['./maintenance.php'])"> | ||
| <a href="#"> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Broken navigation: Maintenance menu link has no functional target.
The onclick="openUrl(['./maintenance.php'])" handler was removed, leaving only href="#". This means clicking the top-level Maintenance menu item now does nothing except scroll to top or toggle the submenu.
While submenu items (lines 377-389) still link to maintenance.php tabs, removing the top-level navigation is inconsistent with:
- The Settings link (line 394) which retains both
href="#"and onclick handler - The active state logic (line 365) that checks
basename($_SERVER['SCRIPT_NAME']) === 'maintenance.php', implying users should be able to navigate to that page - The Devices link (line 291) which now has both href AND onclick
This creates an inconsistent navigation pattern across treeview menu items and potentially prevents direct access to the Maintenance page.
🔧 Proposed fix to restore navigation
Option 1: Restore the onclick handler for consistency with Settings menu:
- <a href="#">
+ <a href="#" onclick="openUrl(['./maintenance.php'])">Option 2: Add proper href for consistency with Devices menu:
- <a href="#">
+ <a href="maintenance.php" onclick="openUrl(['./maintenance.php'])">Option 3: If treeview toggle is the only intended behavior, remove onclick from all treeview parents for consistency (including Settings line 394).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <a href="#"> | |
| <a href="#" onclick="openUrl(['./maintenance.php'])"> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@front/php/templates/header.php` at line 366, The Maintenance menu link at
line 366 in the header.php file has lost its onclick handler, leaving only
href="#", which makes the top-level menu item non-functional. Restore the
onclick="openUrl(['./maintenance.php'])" handler to the anchor tag for the
Maintenance menu item to match the pattern used in the Settings menu link (line
394) and Devices menu link (line 291), ensuring users can navigate directly to
the maintenance.php page and making the navigation pattern consistent across all
treeview menu items.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front/appEventsCore.php (1)
121-127: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMirror UI teardown in the AJAX error path.
Line 121-127 does not call
hideSpinner()(and should also hide the app-events skeleton), so failed requests can leave loading UI stuck.Suggested patch
error: function () { + hideSpinner(); + hideAppEventsSkeleton(); callback({ data: [], recordsTotal: 0, recordsFiltered: 0 }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front/appEventsCore.php` around lines 121 - 127, The error callback function for the AJAX request is not properly tearing down the loading UI when a request fails, leaving the spinner and app-events skeleton visible. Add `hideSpinner()` and hide the app-events skeleton element inside the error callback function (where the callback is invoked with the empty data response) to mirror the UI teardown that should occur on both successful and failed requests, ensuring the loading state is properly cleared regardless of the outcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@front/css/app.css`:
- Around line 2664-2665: The CSS selector `#skel-app-event` (singular) does not
match the actual DOM element id `#skel-app-events` (plural), causing the skeleton
styling rules to not apply to the intended element. Update all instances of the
`#skel-app-event` selector to `#skel-app-events` (add the 's' to make it plural) in
the CSS file to match the DOM id. This selector mismatch occurs in multiple
locations throughout the file where the selector is defined.
---
Outside diff comments:
In `@front/appEventsCore.php`:
- Around line 121-127: The error callback function for the AJAX request is not
properly tearing down the loading UI when a request fails, leaving the spinner
and app-events skeleton visible. Add `hideSpinner()` and hide the app-events
skeleton element inside the error callback function (where the callback is
invoked with the empty data response) to mirror the UI teardown that should
occur on both successful and failed requests, ensuring the loading state is
properly cleared regardless of the outcome.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7fadc81e-92f6-4026-8eba-a8ea7cf24db7
📒 Files selected for processing (10)
front/appEvents.phpfront/appEventsCore.phpfront/css/app.cssfront/css/dark-patch.cssfront/css/system-dark-patch.cssfront/devices.phpfront/php/templates/header.phpfront/php/templates/skel_app_events.phpfront/settings.phpfront/systeminfoNetwork.php
✅ Files skipped from review due to trivial changes (3)
- front/appEvents.php
- front/systeminfoNetwork.php
- front/php/templates/skel_app_events.php
🚧 Files skipped from review as they are similar to previous changes (3)
- front/php/templates/header.php
- front/devices.php
- front/settings.php
1 participant
Summary by CodeRabbit